Skip to content

feat(files_sharing): implement partial mount providers #57292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
salmart-dev wants to merge 9 commits into
base: master
Choose a base branch
from
Open

feat(files_sharing): implement partial mount providers #57292

salmart-dev wants to merge 9 commits into from
+248 −41

Conversation

@salmart-dev
Copy link
Contributor

@salmart-dev salmart-dev commented Dec 30, 2025 •
edited by artonge
Loading

Summary

Implements IPartialMountProvider in MountProvider for external files_sharing.

This PR depends on #57289 and and #57286 and their dependant PRs to be merged to function correctly.

Once this PR is merged, all providers that do not implement IPartialShareProvider will stop being called during the setup and shares will disappear. It is important this is merged after all share providers implement #57285

TODO

  • Add tests
  • Add indexes for target on oc_share and oc_share_external

Checklist

@salmart-dev salmart-dev added this to the Nextcloud 33 milestone Dec 30, 2025
@salmart-dev salmart-dev self-assigned this Dec 30, 2025
@salmart-dev salmart-dev added the 2. developing Work in progress label Dec 30, 2025
@salmart-dev salmart-dev force-pushed the feature/54562/drop-mounts-on-full-or-provider-setup branch 2 times, most recently from 463f8cd to 5de8e11 Compare December 31, 2025 09:04
@salmart-dev salmart-dev force-pushed the feature/54562/files-sharing-authoritative branch 2 times, most recently from 03c02a1 to 63ec2c4 Compare January 2, 2026 10:54
@salmart-dev salmart-dev added 3. to review Waiting for reviews feature: sharing and removed 2. developing Work in progress labels Jan 2, 2026
@salmart-dev salmart-dev marked this pull request as ready for review January 2, 2026 14:54
@salmart-dev salmart-dev requested a review from a team as a code owner January 2, 2026 14:54
@salmart-dev salmart-dev requested review from Altahrim, leftybournes, provokateurin and yemkareems and removed request for a team January 2, 2026 14:54
@CarlSchwan CarlSchwan changed the title feat(files_sharing): implement partial mount providers feat(external-shares): implement partial mount providers Jan 6, 2026
@artonge artonge force-pushed the feature/54562/drop-mounts-on-full-or-provider-setup branch 2 times, most recently from 6a7f493 to d8d172c Compare January 7, 2026 09:07
@nextcloud-bot nextcloud-bot mentioned this pull request Jan 7, 2026
Merged
@artonge artonge force-pushed the feature/54562/drop-mounts-on-full-or-provider-setup branch from d8d172c to 2d22c4f Compare January 7, 2026 16:00
Base automatically changed from feature/54562/drop-mounts-on-full-or-provider-setup to master January 8, 2026 10:26
@CarlSchwan CarlSchwan changed the title feat(external-shares): implement partial mount providers feat(files_sharing): implement partial mount providers Jan 8, 2026
@artonge artonge force-pushed the feature/54562/files-sharing-authoritative branch from 63ec2c4 to efcdd85 Compare January 8, 2026 14:20
@nextcloud-bot nextcloud-bot mentioned this pull request Jan 9, 2026
Merged
@artonge artonge force-pushed the feature/54562/files-sharing-authoritative branch from e9711fb to 4d2a155 Compare January 14, 2026 09:11
@nextcloud-bot nextcloud-bot mentioned this pull request Jan 14, 2026
Open
@artonge artonge force-pushed the feature/54562/files-sharing-authoritative branch from 4d2a155 to 98cd77b Compare January 14, 2026 14:38
@artonge artonge force-pushed the feature/54562/files-sharing-authoritative branch from 98cd77b to 44ce0f0 Compare January 14, 2026 14:41
@icewind1991
Copy link
Member

icewind1991 commented Jan 14, 2026

I think #57550 fixes the test failures here

@artonge
Copy link
Contributor

artonge commented Jan 14, 2026 •
edited
Loading

Questions

  • Should we ignore mountpoints that are not listed in $mountProviderArgs?
  • Do we really need to pass $mountProviderArgs to the getMountsForPath() implementations? Currently, we do not use it.
  • Salavtore changed an index share_with_file_target_index, does it mean that query that do not include file_target won't use the index now? Do we really want that?
  • I move all the shared logic out of the getMountsForPath() implementations to their caller getUserMountsFromProviderByPath(). Is it a good idea? I don't see in which mount provider we would not do the same.

icewind1991
icewind1991 reviewed Jan 14, 2026
View reviewed changes
lib/private/Files/Config/MountProviderCollection.php
Comment on lines +128 to +131
// remove /uid/files as the target is stored without
$path = \substr($path, \strlen('/' . $userId . '/files'));
// remove trailing slash
$path = \rtrim($path, '/');
Copy link
Member

@icewind1991 icewind1991 Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this might hold for many mount providers, it's not required and I think having it as part of the api (by having the provided path be relative) would be a bad idea.

(In general I think passing around relative paths is bad)

icewind1991
icewind1991 reviewed Jan 14, 2026
View reviewed changes
lib/private/Files/Config/MountProviderCollection.php

if (!$forChildren) {
// override path with mount point when fetching without children
$path = $mountProviderArgs[0]->mountInfo->getMountPoint();
Copy link
Member

@icewind1991 icewind1991 Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$path has a specific and separate meaning from $mountProviderArgs[0]->mountInfo->getMountPoint().

If anything, most mount providers can probably ignore the $path argument and should only look at the $mountProviderArgs

Copy link
Contributor

@artonge artonge Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That make sense.
This means that we could use getMountPoint() for file_target instead of doing a like on $path_%?
But this won't work for the Talk folder where we have 1000+ entries.

@icewind1991
Copy link
Member

icewind1991 commented Jan 14, 2026

Salavtore changed an index share_with_file_target_index, does it mean that query that do not include file_target won't use the index now? Do we really want that?

We probably want to also keep the old index

@icewind1991
Copy link
Member

icewind1991 commented Jan 14, 2026

Should we ignore mountpoints that are not listed in $mountProviderArgs?

Returning "to much" should be harmless

@icewind1991
Copy link
Member

icewind1991 commented Jan 14, 2026

Do we really need to pass $mountProviderArgs to the getMountsForPath() implementations? Currently, we do not use it.

It's the part that has the actual "real" data about what mounts to setup, $path is "just" the path for which the setup was requested, not the path for which mounts are expected.

It would also be better for performance for mount providers to use the provided CacheEntry (though weaving that into the logic might be non-trivial)

@icewind1991
Copy link
Member

icewind1991 commented Jan 14, 2026

I move all the shared logic out of the getMountsForPath() implementations to their caller getUserMountsFromProviderByPath(). Is it a good idea? I don't see in which mount provider we would not do the same.

"All mount provider do the same" and "the way they do it should be part of the api" are two different things that I think shouldn't be conflated.

@artonge
Copy link
Contributor

artonge commented Jan 14, 2026

Returning "to much" should be harmless

If we return too much, should we then register those missing mounts?

@icewind1991
Copy link
Member

icewind1991 commented Jan 14, 2026

If we return too much, should we then register those missing mounts?

Probably, determining which of the extra ones are not registered already without extra cost might be tricky though.

@artonge
Copy link
Contributor

artonge commented Jan 14, 2026

Probably, determining which of the extra ones are not registered already without extra cost might be tricky though.

Well, we have the list of registered mount points in $mountProviderArgs (A), and the list of mount point from the provider perspective (B). So anything that is in B and not in A should be registered, no?

@icewind1991
Copy link
Member

icewind1991 commented Jan 14, 2026

Well, we have the list of registered mount points in $mountProviderArgs (A), and the list of mount point from the provider perspective (B). So anything that is in B and not in A should be registered, no?

Providers could return mounts that are registered, just not requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@icewind1991 icewind1991 icewind1991 left review comments

@artonge artonge artonge left review comments

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@Altahrim Altahrim Awaiting requested review from Altahrim Altahrim is a code owner automatically assigned from nextcloud/server-backend

@yemkareems yemkareems Awaiting requested review from yemkareems yemkareems is a code owner automatically assigned from nextcloud/server-backend

@leftybournes leftybournes Awaiting requested review from leftybournes leftybournes is a code owner automatically assigned from nextcloud/server-backend

@provokateurin provokateurin Awaiting requested review from provokateurin provokateurin is a code owner automatically assigned from nextcloud/server-backend

At least 2 approving reviews are required to merge this pull request.

Assignees

@salmart-dev salmart-dev

Labels

3. to review Waiting for reviews feature: sharing

Projects

None yet

Milestone

Nextcloud 33

Development

Successfully merging this pull request may close these issues.

5 participants

@salmart-dev @icewind1991 @artonge @CarlSchwan